Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(frontend): search spans from extension tracers when object name is specified #369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xuqingyun
Copy link
Collaborator

Description

  • when searched object has no events during this period, try to discover any possible spans from extension tracers.

Related issues

Special notes for your reviewer:

@xuqingyun xuqingyun changed the title feat(frontend): search spans from extension tracers when object name is specified wip: feat(frontend): search spans from extension tracers when object name is specified Oct 8, 2024
@xuqingyun xuqingyun force-pushed the git-xqy/search_from_extension branch from df20cd3 to 0e47d68 Compare October 8, 2024 10:17
@xuqingyun xuqingyun changed the title wip: feat(frontend): search spans from extension tracers when object name is specified feat(frontend): search spans from extension tracers when object name is specified Oct 8, 2024
@xuqingyun xuqingyun force-pushed the git-xqy/search_from_extension branch from 0e47d68 to 756f654 Compare October 8, 2024 12:34
@@ -237,14 +237,24 @@ func newObject[M any](key objKey, trace *tftree.SpanTree, metadata M) (*object[M
if err != nil {
return nil, fmt.Errorf("clone spans: %w", err)
}

var m []M
if !isNil(metadata) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNil will always be false unless M is an interface. It is best not to rely on such unexpected behavior.

What problem are you trying to solve here? Is it better to change trace.Metadata itself to a slice?

Copy link
Collaborator Author

@xuqingyun xuqingyun Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SOF3 if trace.Metadata is nil,slice长度为1,identifiers序列化之后变成了字符串null,读缓存的时候没有办法再去判断这个null是个什么东西。 trace.Metadata 改为slice是可以,不过在读取缓存的时候依然有这个风险遇到未知类型。
不过any不就是interface{}吗,所以M不就一定是个interface{},如果不是interface不应该会编译错误?如果是int等基础类型不为空应该是预期内的

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M is struct{} when called from GetTrace(). any(struct{}{}) == nil is false.

Copy link
Member

@SOF3 SOF3 Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it meets your expectations, but checking metadata as nil randomly would be very strange (e.g. if M happens to be a thin pointer in the future, isNil will be false even for nil pointers).

I would prefer fixing this issue at a higher level, e.g. just making the trace.Metadata field an optional field or a slice, or just correcting it in the reader where it knows the real type of M.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, 我直接在读缓存的地方加个兼容吧

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M is struct{} when called from GetTrace(). any(struct{}{}) == nil is false.

不过...这个好像是符合空指针判断逻辑的,这个case就不应该为nil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but when M is *Foo, any((*Foo)(nil)) == nil is also false, which could be confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, from a generic point of view, isNil doesn't serve any semantic purpose, so this logic is quite specific to one of the callers anyway.

@xuqingyun xuqingyun force-pushed the git-xqy/search_from_extension branch from 756f654 to 71760a4 Compare October 9, 2024 06:10
@xuqingyun xuqingyun force-pushed the git-xqy/search_from_extension branch from 71760a4 to e6c150f Compare October 9, 2024 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants